feat: add java.time.OffsetDateTime converters (#1017) by Mikkey-f · Pull Request #1033 · apache/fesod

Mikkey-f · 2026-08-23T06:16:14Z

What and why

Adds a java.time.OffsetDateTime converter family for the OffsetDateTime slice of #1017, following the existing LocalDateTime / ZonedDateTime pattern, so OffsetDateTime fields map to Excel natively instead of falling back to String:

OffsetDateTimeDateConverter — write-only, emits an Excel DATE cell via toLocalDateTime(), default format yyyy-MM-dd HH:mm:ss
OffsetDateTimeNumberConverter — bidirectional NUMBER serial, respects use1904windowing (property-level first, then a null-safe global default); on read attaches ZoneId.systemDefault() to the parsed LocalDateTime
OffsetDateTimeStringConverter — bidirectional STRING, honors @DateTimeFormat and the configured Locale, defaults to ISO_OFFSET_DATE_TIME when no format is set

Registered in DefaultConverterLoader.initAllConverter() / initDefaultWriteConverter().

Tests

OffsetDateTimeConverterTest covers converter keys, DATE/NUMBER/STRING read & write, @DateTimeFormat formatting, use1904windowing (including the null-safe global default) and round-trip behavior. All tests pass, spotless:check is green, and the full local build was verified.

Closes #1017 (OffsetDateTime slice).

Copilot

Pull request overview

Adds built-in OffsetDateTime conversion support for native date, numeric serial, and string Excel cells.

Changes:

Adds DATE, NUMBER, and STRING converters.
Registers converters in default loader maps.
Adds unit tests for formatting, parsing, windowing, and registration.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/OffsetDateTimeConverterTest.java	Updated as part of this pull request.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeStringConverter.java	Updated as part of this pull request.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeNumberConverter.java	Updated as part of this pull request.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/offsetdatetime/OffsetDateTimeDateConverter.java	Updated as part of this pull request.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java	Updated as part of this pull request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Mikkey-f · 2026-08-23T11:06:45Z

+        if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
+            format = contentProperty.getDateTimeFormatProperty().getFormat();
+        }
+        WorkBookUtil.fillDataFormat(cellData, format, DateUtils.defaultDateFormat);


Empty @DateTimeFormat format — Fixed in OffsetDateTimeDateConverter: empty formats are now normalized to null before calling WorkBookUtil.fillDataFormat, so the yyyy-MM-dd HH:mm:ss default is applied. Added a regression test(dateConverterFallsBackToDefaultFormatForEmptyDateTimeFormat).

Mikkey-f · 2026-08-23T11:07:46Z

+        if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
+            Boolean propertyUse1904windowing =
+                    contentProperty.getDateTimeFormatProperty().getUse1904windowing();
+            if (propertyUse1904windowing != null) {
+                return propertyUse1904windowing;


use1904windowing DEFAULT being unboxed to false — Agreed this is a real issue, but it's a pre-existing framework-level problem: DateTimeFormatProperty.build converts BooleanEnum.DEFAULT (null) to false (DateTimeFormatProperty.java:55-57), and all existing number converters (Date, LocalDate, LocalDateTime, ZonedDateTime) consume the property value without a global fallback. This PR's null-safe fallback covers the no-annotation path; the annotated path behaves identically to the existing converter families. Fixing it properly means changing DateTimeFormatProperty (preserving DEFAULT as null) and updating every date-number converter — a framework-wide change that deserves its own issue/PR. Happy to open one if that's useful.

Mikkey-f · 2026-08-23T11:07:04Z

+        String format = format(contentProperty);
+        if (StringUtils.isEmpty(format)) {
+            return DateTimeFormatter.ISO_OFFSET_DATE_TIME;
+        }
+        return DateTimeFormatter.ofPattern(format, locale);


Formatter caching — Fixed in OffsetDateTimeStringConverter: DateTimeFormatter instances are now cached per pattern and locale in a thread-local map, avoiding rebuilds in the per-cell hot path (the ISO default is a shared constant).

bengbengbalabalabeng · 2026-08-23T13:32:44Z

The newly added files in this PR are implemented from scratch and are not derived from Alibaba's EasyExcel. Therefore, no EasyExcel-related license header is required for these files.

Please refer to: https://github.com/apache/fesod/blob/main/fesod-sheet/src/main/java/org/apache/fesod/sheet/FesodSheet.java

bengbengbalabalabeng · 2026-09-07T12:24:45Z

+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;


Avoid using wildcard imports.

Thanks for the comment. The imports this is anchored to (lines 22-25, after the header cleanup: assertEquals, assertNotNull, assertNull, assertThrows) are explicit static imports — each imports a single method, and all four are used in the tests. There is no import static org.junit.jupiter.api.Assertions.*; style wildcard anywhere in the files changed by this PR.

If you mean these four lines should not be collapsed into a wildcard import — current code already follows that. If you'd rather I drop the static imports entirely and use fully-qualified Assertions.xxx calls, I'm happy to do that — just confirm and I'll update.

Apologies for the typo. The intended phrase was "avoid static imports".

Thanks for the clarification — I've replaced all static Assertions imports in OffsetDateTimeConverterTest with fully-qualified Assertions.xxx calls (16 call sites, plus the import). The other files in this PR had no static imports. Updated and pushed.

bengbengbalabalabeng · 2026-09-07T12:26:22Z

The newly added files in this PR are implemented from scratch and are not derived from Alibaba's EasyExcel. Therefore, no EasyExcel-related license header is required for these files.

Please refer to: https://github.com/apache/fesod/blob/main/fesod-sheet/src/main/java/org/apache/fesod/sheet/FesodSheet.java

Please update the license header.

Mikkey-f · 2026-09-07T14:07:56Z

Thanks for the review. You're right — these files were implemented from scratch, so I've removed the EasyExcel-derived header block (including the Alibaba copyright notice) from the three new converters and the test file. They now carry only the standard ASF header, matching the convention in FesodSheet.java and the LocalTime converters merged in #1032.

Add OffsetDateTimeStringConverter, OffsetDateTimeNumberConverter and OffsetDateTimeDateConverter, following the existing LocalDateTime and ZonedDateTime converter patterns: - String conversion preserves the offset in ISO-8601 text by default, with a configurable pattern, falling back to local wall-clock time when the offset is missing. - Number and date conversions drop the offset while preserving the local wall-clock time, consistent with the ZonedDateTime converters.

- Read fallback now routes through DateUtils.parseLocalDateTime so the default space-separated format written by other date converters is accepted, and text that does not match a configured pattern is rejected. - Return null instead of NPE for invalid Excel serials, matching the LocalDateTime family. - Null-safe use1904windowing resolution and default-locale fallback.

…erters (apache#1017)

Address review comments: - OffsetDateTimeDateConverter: an empty @DateTimeFormat value bypassed WorkBookUtil.fillDataFormat's default format (only null falls back), writing an empty/General number format instead of yyyy-MM-dd HH:mm:ss. Normalize empty formats to null; regression test added. - OffsetDateTimeStringConverter: cache DateTimeFormatter instances per pattern and locale in a thread-local map instead of rebuilding them on every cell conversion in the hot path.

…converters (apache#1017)

…nverter (apache#1017)

…s in OffsetDateTimeConverterTest (apache#1017)

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from 347d5b5 to b9122be Compare August 23, 2026 06:26

delei added the PR: first-time contributor first-time contributor label Aug 23, 2026

delei requested a lite review from Copilot August 23, 2026 10:18

Copilot started reviewing on behalf of delei August 23, 2026 10:19 View session

Copilot AI reviewed Aug 23, 2026

View reviewed changes

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from 3d00856 to 7dd544e Compare August 25, 2026 10:13

Mikkey-f mentioned this pull request Aug 28, 2026

fix: fall back to global 1904 windowing when @DateTimeFormat default is used (#1042) #1054

Merged

bengbengbalabalabeng reviewed Sep 7, 2026

View reviewed changes

Mikkey-f added 6 commits September 7, 2026 22:14

fix: null-safe global use1904windowing default in OffsetDateTime conv…

36b6278

…erters (apache#1017)

fix: remove EasyExcel-derived license header from new OffsetDateTime …

11184ae

…converters (apache#1017)

refactor: reuse shared DateUtils.isDate1904 in OffsetDateTimeNumberCo…

255c63a

…nverter (apache#1017)

Mikkey-f force-pushed the feat/offsetdatetime-converters branch from 4fb6929 to 255c63a Compare September 7, 2026 14:19

refactor: replace static Assertions imports with fully-qualified call…

e5d642a

…s in OffsetDateTimeConverterTest (apache#1017)

Navigation Menu

Uh oh!

feat: add java.time.OffsetDateTime converters (#1017) - #1033

feat: add java.time.OffsetDateTime converters (#1017)#1033
Mikkey-f wants to merge 7 commits into
apache:mainfrom
Mikkey-f:feat/offsetdatetime-converters

Mikkey-f commented Aug 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Mikkey-f Aug 23, 2026

Uh oh!

Mikkey-f Aug 23, 2026

Uh oh!

Mikkey-f Aug 23, 2026

Uh oh!

bengbengbalabalabeng commented Aug 23, 2026

Uh oh!

bengbengbalabalabeng Sep 7, 2026

Uh oh!

Mikkey-f Sep 7, 2026

Uh oh!

bengbengbalabalabeng Sep 7, 2026

Uh oh!

Mikkey-f Sep 7, 2026

Uh oh!

bengbengbalabalabeng commented Sep 7, 2026

Uh oh!

Mikkey-f commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Mikkey-f commented Aug 23, 2026

What and why

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Mikkey-f Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

Mikkey-f Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

Mikkey-f Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

bengbengbalabalabeng commented Aug 23, 2026

Uh oh!

bengbengbalabalabeng Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Mikkey-f Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

bengbengbalabalabeng Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Mikkey-f Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

bengbengbalabalabeng commented Sep 7, 2026

Uh oh!

Mikkey-f commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants